Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

enh: allow refresh times greater than max JS number, of ~24.8 days #891

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

enh: allow refresh times greater than max JS number, of ~24.8 days #891

wants to merge 11 commits into from

Conversation

cip8
Copy link
Contributor

@cip8 cip8 commented Aug 29, 2024

🔗 Linked issue

889 (link)

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

  • Postponing refresh when interval is greater than 24.8 days, to comply with JS max number limit.
  • Increased default refresh interval - 5 mins is a reasonable minimum for most apps & avoids too many calls to backend.

📝 Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

cip8 added 3 commits August 29, 2024 14:45
@cip8
Postponing refresh when interval is greater than 24.8 days, to comply…
c8fc291 
… with JS max number limit.
@cip8
Increased default refresh interval - 5 mins is a reasonable minimum f…
f3b0b1c 
…or most apps & avoids too many calls to backend.
@cip8
Fixed timer refresh warning - this.maxAgeMs should always be set when…
40a6674 
… the method is called, but we default to zero just in case.
@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Aug 29, 2024 •
edited
Loading

Open in Stackblitz

pnpm add https://pkg.pr.new/@sidebase/nuxt-auth@891

commit: f49294d

@cip8
Copy link
Contributor Author

cip8 commented Aug 29, 2024 •
edited
Loading

Just realized I need to apply the solution for refetchIntervalTimer too - both for consistency and to cover cases where users might prefer to also set enablePeriodically to a very long interval.

Will update this PR soon.

@cip8
Copy link
Contributor Author

cip8 commented Aug 29, 2024

Done - tested with my backend, everything works fine.

Let me know if it needs anything else!

@zoey-kaiser
Copy link
Member

Hi @cip8 👋

Thank you for the PR! I would propose @phoenix-ru does a full review when he gets back from his break, as he wrote / maintains the refresh logic.

Until then feel free to install and use the temporary package created with your last commit as a fix until the PR is merged and released:

pnpm add https://pkg.pr.new/@sidebase/nuxt-auth@b41f424

@zoey-kaiser zoey-kaiser requested a review from phoenix-ru August 29, 2024 15:29
@zoey-kaiser zoey-kaiser added enhancement An improvement that needs to be added provider-refresh An issue with the refresh provider labels Aug 29, 2024
@cip8 cip8 mentioned this pull request Aug 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phoenix-ru phoenix-ru Awaiting requested review from phoenix-ru

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement An improvement that needs to be added provider-refresh An issue with the refresh provider
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cip8 @zoey-kaiser